New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(spanner/spansql): support table_hint_expr at from_clause on query_statement #4457
Conversation
query_statement This fixes parse error when query statement includes table hint expr. This add function parseHints and use in parsing table hints and join hints.
@olavloite Can you please review this PR? Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution. This looks mostly good to me, but it seems that the documentation is not 100% correct on whether multiple table hints are allowed or not. Cloud Spanner actually does support more than one table hint, so we should try to implement it in the same way in this emulator. See my comments and example below.
spanner/spansql/parser.go
Outdated
@@ -2111,6 +2111,13 @@ func (p *parser) parseSelectFrom() (SelectFrom, *parseError) { | |||
return nil, err | |||
} | |||
sf := SelectFromTable{Table: tname} | |||
if p.eat("@") { | |||
hint, err := p.parseHints(map[string]string{}, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... It seems that the documentation is slightly off here. It is actually possible to have more than one table hint, so the variable here should be renamed to hints
and the parseHints(..)
function should be called with true
rather than false
.
For example the following query is accepted by Cloud Spanner:
select *
from albums@{FORCE_INDEX=_BASE_TABLE, GROUPBY_SCAN_OPTIMIZATION=FALSE}
spanner/spansql/parser.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
sf.Hint = hint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: same as above, rename to sf.Hints
spanner/spansql/sql.go
Outdated
if len(sft.Hint) > 0 { | ||
for k, v := range sft.Hint { | ||
str += fmt.Sprintf("@{%s=%s}", k, v) | ||
// table hint exists only one key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, it seems that the documentation is not completely clear on this, but Cloud Spanner does accept more than one table hint.
spanner/spansql/types.go
Outdated
@@ -394,6 +394,7 @@ type SelectFrom interface { | |||
type SelectFromTable struct { | |||
Table ID | |||
Alias ID // empty if not aliased | |||
Hint map[string]string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: rename to Hints
Thanks for your kind review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM
@hengfengli I think we need an approval from either you or @skuruppu in order to merge this. |
The integration test fails due to #4450. I will merge this PR after it is merged. |
@nakatamixi Thank you for your contribution! |
Thank you! |
googleapis#4457 made unstable SQL string, and fail test sometimes like this. ``` --- FAIL: TestSQL (0.00s) sql_test.go:410: {{false [A] [{Table map[FORCE_INDEX:Idx GROUPBY_SCAN_OPTIMIZATION:TRUE]}] {4 B b <nil>} [] [] []} [] <nil> <nil>}.SQL() wrong. got SELECT A FROM Table@{GROUPBY_SCAN_OPTIMIZATION=TRUE,FORCE_INDEX=Idx} WHERE B = @b want SELECT A FROM Table@{FORCE_INDEX=Idx,GROUPBY_SCAN_OPTIMIZATION=TRUE} WHERE B = @b ``` Because map range result is unstable. This PR makes the SQL output stable by sorting the Hints keys.
googleapis#4457 made unstable SQL string, and fail test sometimes like this. ``` --- FAIL: TestSQL (0.00s) sql_test.go:410: {{false [A] [{Table map[FORCE_INDEX:Idx GROUPBY_SCAN_OPTIMIZATION:TRUE]}] {4 B b <nil>} [] [] []} [] <nil> <nil>}.SQL() wrong. got SELECT A FROM Table@{GROUPBY_SCAN_OPTIMIZATION=TRUE,FORCE_INDEX=Idx} WHERE B = @b want SELECT A FROM Table@{FORCE_INDEX=Idx,GROUPBY_SCAN_OPTIMIZATION=TRUE} WHERE B = @b ``` Because map range result is unstable. This PR makes the SQL output stable by sorting the Hints keys.
#4457 made unstable SQL string, and fail test sometimes like this. Because map range result is unstable. This PR makes the SQL output stable by sorting the Hints keys.
This fixes parse error when query statement includes table hint expr.
This add function
parseHints
and use in parsing table hints and join hints.